Skip to content

[DEVEX-222] - Auto serialization feedback improvements #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

oskardudycz
Copy link
Collaborator

@oskardudycz oskardudycz commented Mar 12, 2025

This PR follows up on the initial PR that introduced auto-serialization and applies the discussed changes.

  1. Brought back expected stream state to append to stream methods. Even if users may decide to ignore optimistic concurrency check by using StreamState.Any and it may be a valid decision, we want to promote using optimistic concurrency and making it a cautious decision to resign from it.

  2. Introduced of OperationOptions, which wraps common settings like deadline and credentials. I used it as the base class for reading, appending, and internal implementations. That's also aligned with the current Java client approach. This can be also preparation for future changes like batch appends.

  3. Introduced MessageData and used it instead of EventData. As we want to promote KurrentDB as a general message store, and introduced Message type as a wrapper for the serialization then it's a logical step. Made EventData be possible to cast to message map to make easier migration and marked it as obsolete. Also applied some usability improvements in those classes (boy scout rule).

  4. Added raw append to stream with options to give people a clear path when they want to provide raw bytes. Thanks to that, other append methods with dozens of parameters were obsolete and moved to extension methods. This should be less confusing, as now we have two official paths: auto-serialization and raw appends. The rest are clearly stated as obsolete.

  5. Added DeserializedData and DeserializedMessages extension methods for read results. Thanks to that, people can easily map message data if they don't want to use the full ResolvedEvent. That makes clearer usage path for folks that offload all serialization and message mapping to us. You can use it as:

List<Message> deserializedMessages = await kurrentDbClient
    .ReadStreamAsync(stream)
    .DeserializedMessages()
    .ToListAsync();
		
List<object> deserializedDomainMessages = await kurrentDbClient
    .ReadStreamAsync(stream)
    .DeserializedData()
    .ToListAsync();
  1. Besides that, simplified the MessageSerializer. Now it creates a derived serializer merging operation settings with default settings if needed. That allowed hiding internal details, and there's no need to pass content type explicitly, as it's already in settings.
    Extended SchemaRegistry will provide wrapper methods for naming the resolution context. Thanks to that MessageSerializer doesn't need to reference it internally.
    To make responsibilities clearer, made also naming resolution context part of the serializer context.

  2. Aligned metadata serializer with the default, system one. Those settings will be different than the ones in regular data, but this will ensure that changes are backward compatible to tracing, etc, made by existing users.

TODO:

  • Make old read methods obsolete,
  • Make old subscription methods obsolete.

@oskardudycz oskardudycz changed the base branch from master to DEVEX-185-Rebranding March 12, 2025 10:55
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch 3 times, most recently from 0f2f0be to 23ed48e Compare March 13, 2025 08:33
@oskardudycz oskardudycz changed the base branch from DEVEX-185-Rebranding to yoeight/new-stream-state March 13, 2025 10:56
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch 3 times, most recently from e7adb5b to abccac0 Compare March 13, 2025 13:31
@YoEight YoEight force-pushed the yoeight/new-stream-state branch from 82962d1 to 676cc36 Compare March 17, 2025 13:14
Copy link
Contributor

@yreynhout yreynhout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me

@@ -195,7 +187,7 @@ IWriteResult HandleWrongExpectedRevision(
actualStreamRevision
);

if (operationOptions.ThrowOnAppendFailure) {
if (operationOptions.ThrowOnAppendFailure == true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but had me go "why?". Did it become nullable perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and no 🙂

Yes because ThrowOnAppendFailure indeed is nullable now, as it can be provided or not by the user. I prefer to keep it nullable to have clear information whether the user sets it or not.

No because it's different OperationSettings than before. Previously we used here KurrentDBClientOperationOptions, which are general settings for the client, and it contains various options that may not be related to specific operations. Previously they were passed even to read operations, where ThrowOnAppendFailure wouldn't make any sense. I replaced them with more explicit and contextual AppendToStreamOptions and generic OperationOptions.

/// <param name="userCredentials">The optional <see cref="UserCredentials"/> to perform operation with.</param>
/// <param name="cancellationToken">The optional <see cref="System.Threading.CancellationToken"/>.</param>
/// <returns></returns>
public static Task<StreamMetadataResult> GetStreamMetadataAsync(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the intent for these to carry an [<Obsolete(...)>] attribute?

Copy link
Collaborator Author

@oskardudycz oskardudycz Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good call! I forgot to mark them as obsolete. Did that in 1a8e507.

@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch 2 times, most recently from 9eefd3a to db4fc93 Compare March 20, 2025 11:21
@oskardudycz oskardudycz changed the base branch from yoeight/new-stream-state to DEVEX-185-Rebranding March 20, 2025 11:24
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch 2 times, most recently from 4e1cf75 to 7815a6f Compare March 20, 2025 13:39
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch from 47a3cff to 1c489b8 Compare March 21, 2025 12:57
@oskardudycz oskardudycz marked this pull request as ready for review March 24, 2025 07:37
@oskardudycz oskardudycz changed the title [WIP] DEVEX-222 - Auto serialization feedback improvements DEVEX-222 - Auto serialization feedback improvements Mar 24, 2025
@oskardudycz oskardudycz changed the title DEVEX-222 - Auto serialization feedback improvements [DEVEX-222] - Auto serialization feedback improvements Mar 24, 2025
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch 3 times, most recently from 95eb311 to f690270 Compare March 24, 2025 08:57
…stream

Added also override for raw event append that has options and made other methods obsolete.

Introduced OperationOptions that compose default client operation settings plus credentials and deadlines.
Added also factor for event data that's not requiring to provide id immediately
…r EventData, and better aligned with general usage of messages instead of just events
…s for read results

That should be a middle ground without forcing people to use a regular ResolvedEvent if they don't want to
Now serializer creates a derived serializer merging operation settings with default settings if needed. That allowed to hide internal details and there's no need to pass content type explicitly, as it's already in settings.

Extended SchemaRegistry to provide wrapper methods to naming resolution context. Thanks to that MessageSerializer doesn't need to reference it internally.

To make responsibilities clearer made also naming resolution context part of serializer context.
…, so e.g. FromEnd assigns also backwards if it wasn't set
@oskardudycz oskardudycz force-pushed the DEVEX-222-AutoSerialization-feedback-improvements branch from f690270 to 6d48b9c Compare March 24, 2025 12:45
…tead of just raw string.

That should enable current users to override their strategy if they used something more than just message type name from EventRecord.

It needs to be EventRecord and not ResolvedEvent, as resolution happens before creating ResolvedEvent.
@CLAassistant
Copy link

CLAassistant commented Mar 27, 2025

CLA assistant check
All committers have signed the CLA.

Tha makes naming strategy focused on just mapping types back and forth and allowing to disable auto registration
@jasonmitchell jasonmitchell merged commit 18b26d9 into DEVEX-185-Rebranding Apr 23, 2025
66 checks passed
@jasonmitchell jasonmitchell deleted the DEVEX-222-AutoSerialization-feedback-improvements branch April 23, 2025 11:52
RagingKore added a commit that referenced this pull request Apr 23, 2025
* moved all files into EventStore.Client project

* moved files into main project and deleted remaining empty projects
dropped support for net6.0 and net7.0
added some helpers to fluentdocker
playing around the idea of keeping the container on tests

* Remove unused target frameworks and pass certificate in healthcheck

* Tests

* Rename test symbols

* Test workflow

* Testing something

* More tests

* Improve CI

* Improve CI

* Fixup

* Publish actions

* Temporarily disable Public actions

* Fixup

* Test warmup

* [DEVEX-222] Add built-in auto-serialization (#329)

* [DEVEX-222] Added configuration options for KurrentClientSettings Create method

This should enable easier customisation like changing serializer settings.

* [DEVEX-222] Added overloads of Append methods that take regular events

They currently use dummy serialisation

* [DEVEX-222] Added TryDeserialize method to ResolvedEvent

Currently it's implemented in a dummy way

* [DEVEX-222] Added first working, but not yet complete JSON built-in serialization

It's not fully ready, as it has hardcoded schema serializer. Main question will be how much from schema registry I need to move here.

* [DEVEX-222] Added autoserialization settings and removed serializer from ResolvedEvent

Per @RagingKore suggestion it'll be better not to keep the reference to serializer in ResolvedEvent to keep the clear intention behind it.

Now, the serializer is resolved from schema registry based on the auto-serialization settings.

Made possible to register more than one serializer per content type. Currently it doesn't support more schema definition types. To be discussed how to include them later on.

* [DEVEX-222] Made serialization settings to create specific serializers

That will make resolution easier and give users ability to inject their own serializers implementation

* [DEVEX-222] Added override of the serialization settings to Persistent Subscriptions

* [DEVEX-222] Added serialization type and merged serialization into Serialization Context

Previously we had DeserializationContext, but after consideration, it'll be better to merge those concepts to make easier customizations per operations (e.g. append, subscribe, etc.).

* [DEVEX-222] Added message type name resolution strategy

Refactored the code accordingly. It takes the full object instead of the limited number of parameters, as you may be using metadata to get parameters about clrtype.

* [DEVEX-222] Refactored Event Type Mapper to not be responsible for resolution of CLR types

* [DEVEX-222] Added automatic message clr type resolution

* [DEVEX-222] Added metadata extensions to allow injecting type informations

* [DEVEX-222] Made CLR type be resolved based on the message type metadata header

* [DEVEX-222] Added the MessageTypeResolutionStrategyWrapper for caching resolved types

* [DEVEX-222] Added MessageSerializationContext to pass additional information like category name

* [DEVEX-222] Refactored SerializationContext into MessageSerializerWrapper to make clearer responsibilities

Actually, it's just wrapping message serializer based on the client settings.

* [DEVEX-222] Added message type maps registration

* [DEVEX-222] Added helpers for custom type resolutions strategy

Removed also obsolete SerializationSettings

* [DEVEX-222] Replaced MessageSerializerWrapper usage with more generic IMessageSerializer

There's no need to force all code to know about wrapper existence, it's okay to just create it during the setup.

* [DEVEX-222] Made Message to be record instead of Struct

* [DEVEX-222] Refactored new AppendToStreamAsync method to have simplified syntax

Now they don't require all the fancy, but redundant in most cases Stream Positions, Directions, EventData etc.

* [DEVEX-222] Refactored Reading events signatures to use options instead of parameters

Thanks to that safe defaults are already provided

* [DEVEX-222] Renamed MessageTypeResolutionStrategy to MessageTypeNamingStrategy

* [DEVEX-222] Made old Read methods be wrapper of the new one, instead the other way round

* [DEVEX-222] Added serialization configuration overrides

Old append and read methods won't be doing default serialization by default. The new one will use auto-seriailization and allow to customize or disable it.

* [DEVEX-222] Removed storing CLR type in metadata and moved it to message type name

Now the FullTypeName is placed in the event type name. Not perfect but here it is.

* [DEVEX-222] Added possibility to register types for category name

Thanks to that they will be resolved automatically by the registered convention. Then user doesn't need to provide the string name by hand.

Added MessageTypeNamingResolution context besides the MessageSerializationContext as it's orthogonal and just overlapping context.

* [DEVEX-222] Added SubscribeToAllOptions and Subscription Listener

This is a similar API as we already have in Java client, to simplify the handlers registration.

* [DEVEX-222] Addes SubscribeToStreamOptions and applied it accordingly to subscirbe to all changes

* [DEVEX-222] Renamed DeserializedEvent to Message in ResolvedEvent

I think that in the longer term, this will make intention clearer

* [DEVEX-222] Added persistent subscription litener and capability to customize serialization for Persistent Subscriptions

* [DEVEX-222] Extended Serialization tests for Appends and Reads

* [DEVEX-222] Added capability to automatically deserialize message metadata

By default it'll try to resolve the TraceMetadata, it can be override with the general type or using custom resolution strategy.

* [DEVEX-222] For unknown reason the reverse Enumerable.Reverse wasn't taken but Array.Reverse 🤷

* [DEVEX-222] Fixed type resolution to correclty find types by assembly

* [DEVEX-222] Added tests for serialization compatibility between manual and automatic deserialization

* [DEVEX-222] Added tests for appends with autoserialization

* Revert "[DEVEX-222] Added tests for appends with autoserialization"

This reverts commit 04e0685.

I'm reverting it as old methods are wrappers for new ones, so the test scenarios are covered by the old test suite.

* [DEVEX-222] Extended serialization test suite

* [DEVEX-222] Added tests for auto-serialization in Catch-up subscriptions

* [DEVEX-222] Added tests for auto-serialization in persistent subscriptions

* [DEVEX-222] Added Expected prefix to StreamState and StreamRevision in AppendToStreamOptions

* [DEVEX-222] Merged namespaces

Fixed also XML docs comments

* [DEVEX-222] Renamed NulloMessageSerializer to NullMesageSerializer

* [DEVEX-222] Added AppendToStream methods that has expected stream revision as a parameter

* [DEVEX-222] Added XML documentation for Serialization settings

* [DEVEX-222] Added Unit tests

* [DEVEX-222] Added tests for type resolution

Used also two external assemlies
- one that's loaded,
- one that's never loaded.

To double-check behaviour of loading types from different assemblies.

* [DEVEX-222] Added SchemaRegistry tests

* Revert "[DEVEX-222] Add built-in auto-serialization (#329)" (#332)

This reverts commit 44607ea.

* [DEVEX-222] Add built-in auto-serialization (#333)

* [DEVEX-222] Added configuration options for KurrentClientSettings Create method

This should enable easier customisation like changing serializer settings.

* [DEVEX-222] Added overloads of Append methods that take regular events

They currently use dummy serialisation

* [DEVEX-222] Added TryDeserialize method to ResolvedEvent

Currently it's implemented in a dummy way

* [DEVEX-222] Added first working, but not yet complete JSON built-in serialization

It's not fully ready, as it has hardcoded schema serializer. Main question will be how much from schema registry I need to move here.

* [DEVEX-222] Added autoserialization settings and removed serializer from ResolvedEvent

Per @RagingKore suggestion it'll be better not to keep the reference to serializer in ResolvedEvent to keep the clear intention behind it.

Now, the serializer is resolved from schema registry based on the auto-serialization settings.

Made possible to register more than one serializer per content type. Currently it doesn't support more schema definition types. To be discussed how to include them later on.

* [DEVEX-222] Made serialization settings to create specific serializers

That will make resolution easier and give users ability to inject their own serializers implementation

* [DEVEX-222] Added override of the serialization settings to Persistent Subscriptions

* [DEVEX-222] Added serialization type and merged serialization into Serialization Context

Previously we had DeserializationContext, but after consideration, it'll be better to merge those concepts to make easier customizations per operations (e.g. append, subscribe, etc.).

* [DEVEX-222] Added message type name resolution strategy

Refactored the code accordingly. It takes the full object instead of the limited number of parameters, as you may be using metadata to get parameters about clrtype.

* [DEVEX-222] Refactored Event Type Mapper to not be responsible for resolution of CLR types

* [DEVEX-222] Added automatic message clr type resolution

* [DEVEX-222] Added metadata extensions to allow injecting type informations

* [DEVEX-222] Made CLR type be resolved based on the message type metadata header

* [DEVEX-222] Added the MessageTypeResolutionStrategyWrapper for caching resolved types

* [DEVEX-222] Added MessageSerializationContext to pass additional information like category name

* [DEVEX-222] Refactored SerializationContext into MessageSerializerWrapper to make clearer responsibilities

Actually, it's just wrapping message serializer based on the client settings.

* [DEVEX-222] Added message type maps registration

* [DEVEX-222] Added helpers for custom type resolutions strategy

Removed also obsolete SerializationSettings

* [DEVEX-222] Replaced MessageSerializerWrapper usage with more generic IMessageSerializer

There's no need to force all code to know about wrapper existence, it's okay to just create it during the setup.

* [DEVEX-222] Made Message to be record instead of Struct

* [DEVEX-222] Refactored new AppendToStreamAsync method to have simplified syntax

Now they don't require all the fancy, but redundant in most cases Stream Positions, Directions, EventData etc.

* [DEVEX-222] Refactored Reading events signatures to use options instead of parameters

Thanks to that safe defaults are already provided

* [DEVEX-222] Renamed MessageTypeResolutionStrategy to MessageTypeNamingStrategy

* [DEVEX-222] Made old Read methods be wrapper of the new one, instead the other way round

* [DEVEX-222] Added serialization configuration overrides

Old append and read methods won't be doing default serialization by default. The new one will use auto-seriailization and allow to customize or disable it.

* [DEVEX-222] Removed storing CLR type in metadata and moved it to message type name

Now the FullTypeName is placed in the event type name. Not perfect but here it is.

* [DEVEX-222] Added possibility to register types for category name

Thanks to that they will be resolved automatically by the registered convention. Then user doesn't need to provide the string name by hand.

Added MessageTypeNamingResolution context besides the MessageSerializationContext as it's orthogonal and just overlapping context.

* [DEVEX-222] Added SubscribeToAllOptions and Subscription Listener

This is a similar API as we already have in Java client, to simplify the handlers registration.

* [DEVEX-222] Addes SubscribeToStreamOptions and applied it accordingly to subscirbe to all changes

* [DEVEX-222] Renamed DeserializedEvent to Message in ResolvedEvent

I think that in the longer term, this will make intention clearer

* [DEVEX-222] Added persistent subscription litener and capability to customize serialization for Persistent Subscriptions

* [DEVEX-222] Extended Serialization tests for Appends and Reads

* [DEVEX-222] Added capability to automatically deserialize message metadata

By default it'll try to resolve the TraceMetadata, it can be override with the general type or using custom resolution strategy.

* [DEVEX-222] For unknown reason the reverse Enumerable.Reverse wasn't taken but Array.Reverse 🤷

* [DEVEX-222] Fixed type resolution to correclty find types by assembly

* [DEVEX-222] Added tests for serialization compatibility between manual and automatic deserialization

* [DEVEX-222] Added tests for appends with autoserialization

* Revert "[DEVEX-222] Added tests for appends with autoserialization"

This reverts commit 04e0685.

I'm reverting it as old methods are wrappers for new ones, so the test scenarios are covered by the old test suite.

* [DEVEX-222] Extended serialization test suite

* [DEVEX-222] Added tests for auto-serialization in Catch-up subscriptions

* [DEVEX-222] Added tests for auto-serialization in persistent subscriptions

* [DEVEX-222] Added Expected prefix to StreamState and StreamRevision in AppendToStreamOptions

* [DEVEX-222] Merged namespaces

Fixed also XML docs comments

* [DEVEX-222] Renamed NulloMessageSerializer to NullMesageSerializer

* [DEVEX-222] Added AppendToStream methods that has expected stream revision as a parameter

* [DEVEX-222] Added XML documentation for Serialization settings

* [DEVEX-222] Added Unit tests

* [DEVEX-222] Added tests for type resolution

Used also two external assemlies
- one that's loaded,
- one that's never loaded.

To double-check behaviour of loading types from different assemblies.

* [DEVEX-222] Added SchemaRegistry tests

---------

Co-authored-by: Oskar Dudycz <[email protected]>

* Rename namespaces (#335)

* Rename namespaces

* feat: add ability to manually dispatch publish workflow

* chore: set tag prefix to `kurrent@`

* chore: set minimum-major-minor to 1.0

* chore: change name to `Kurrent Inc`

* disable minver

* refactor: move ExpectedRevision to StreamState. (#337)

* [DEVEX-185] Removed leftover folders that had KurrentDb instead of KurrentDB (#340)

It seems that after conflicts when merging StreamRevision PR, the code before merging stayed as duplicated without capital B in KurrentDb.

I checked by doing copying with override from those folders to the proper one, and there were no differences besides those stream revision changes, so it's safe to just remove them.

* chore: update license and readme

* [DEVEX-222] - Auto serialization feedback improvements (#338)

* [DEVEX-222] Fixed the missing application of custom serialization setting in AppendToStreamAsync method

* [DEVEX-222] Added stream state as a mandatory parameter in append to stream

Added also override for raw event append that has options and made other methods obsolete.

Introduced OperationOptions that compose default client operation settings plus credentials and deadlines.

* [DEVEX-222] Ensured that metadata is serialized with auto-serialization as regular system metadata

* [DEVEX-222] Adjusted samples to match the append api changes

Added also factor for event data that's not requiring to provide id immediately

* [DEVEX-222] Added MessageData type as an easier to use replacement for EventData, and better aligned with general usage of messages instead of just events

* [DEVEX-222] Refactored setting stream metadata to use OperationOptions

* [DEVEX-222] Added DeserializedData and DeserializedMessages extensions for read results

That should be a middle ground without forcing people to use a regular ResolvedEvent if they don't want to

* [DEVEX-222] Simplified the message serializer implementation

Now serializer creates a derived serializer merging operation settings with default settings if needed. That allowed to hide internal details and there's no need to pass content type explicitly, as it's already in settings.

Extended SchemaRegistry to provide wrapper methods to naming resolution context. Thanks to that MessageSerializer doesn't need to reference it internally.

To make responsibilities clearer made also naming resolution context part of serializer context.

* [DEVEX-222] Marked old Stream Metadata options as obsolete

* [DEVEX-222] Made old Read methods obsolete and used the new ones in tests

* [DEVEX-222] Made Read options nullable, to precisely know if user provided options or not

* [DEVEX-222] Made the Read Options nullable and thanks to that smarter, so e.g. FromEnd assigns also backwards if it wasn't set

* [DEVEX-222] Made old SetStreamMetadata and ConditionalAppend methods obsolete

* [DEVEX-222] Added delete method with options

* [DEVEX-222] Made old subscription methods obsolete and adjusted tests to use them

* [DEVEX-222] Added Tombstone method with options and made the old obsolete

* [DEVEX-222] Made all persistent subscription methods obsolete

* [DEVEX-222] Added samples showcasing auto-serialization usage

* [DEVEX-222] Reshaped type resolution strategy to take EventRecord instead of just raw string.

That should enable current users to override their strategy if they used something more than just message type name from EventRecord.

It needs to be EventRecord and not ResolvedEvent, as resolution happens before creating ResolvedEvent.

* [DEVEX-222] Changed message type registration to allow multiple message type names mapping to the same CLR class

* [DEVEX-222] Nested Message type mapping into settings object

* [DEVEX-222] Added nested message type mapping

* [DEVEX-222] Replaced clr type with clr type name in naming resolution

Tha makes naming strategy focused on just mapping types back and forth and allowing to disable auto registration

* chore: point workflows to kurrent-io docker images

---------

Co-authored-by: William Chong <[email protected]>
Co-authored-by: Oskar Dudycz <[email protected]>
Co-authored-by: Yo Eight <[email protected]>
Co-authored-by: Oskar Dudycz <[email protected]>
Co-authored-by: Jason Mitchell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants